Skip to content

WW-5698 Scope the ModelDriven exemption in StrutsParameterAuthorizer to the model object - #1872

Open
lukaszlenart wants to merge 5 commits into
mainfrom
feature/WW-5698-modeldriven-exemption-scope
Open

WW-5698 Scope the ModelDriven exemption in StrutsParameterAuthorizer to the model object#1872
lukaszlenart wants to merge 5 commits into
mainfrom
feature/WW-5698-modeldriven-exemption-scope

Conversation

@lukaszlenart

@lukaszlenart lukaszlenart commented Aug 27, 2026

Copy link
Copy Markdown
Member

Fixes WW-5698

Targets 7.4.0. This is a behavioural change (see Compatibility), and requireAnnotations.transitionMode is the supported migration path for applications it affects — that turned out not to be a separate gating decision, because transition mode already is that lever once wired correctly (see the transition mode section).

Problem

StrutsParameterAuthorizer.isAuthorized(...) returned true for any parameter name once the action implemented ModelDriven:

if (target != action && action instanceof ModelDriven) {
    return true;
}

OGNL then resolves that name against the whole CompoundRoot, which holds the model on top of the action. Authorization was decided about the model while the write could land on the action. The practical result: the @StrutsParameter requirement did not apply to a ModelDriven action's own members.

Same unannotated setter, declared on the action class in both cases, with struts.parameters.requireAnnotations=true:

plain action        actionSecret=... -> not bound   (correctly rejected)
ModelDriven action  actionSecret=... -> bound

Change

The exemption now covers what it was meant to cover:

  • a property declared by the model is exempt — returning an object from getModel() declares it request surface, and that is the whole point of the exemption
  • a property declared by the action is subject to the annotation requirement, as it would be on any other action
  • a property declared by neither is still allowed

That third case matters for compatibility. A model bound through a custom OGNL property accessor — a Map-backed model, most commonly — declares no bean property, and such a name cannot be reaching a member of the action either. Rejecting it would break those applications, so it is explicitly allowed.

The model is checked first, so a model property that shadows an action property still binds without an annotation, matching OGNL's own resolution against the stack top.

Compatibility

An application whose ModelDriven action relies on binding unannotated members declared on the action will stop binding them and will need those members annotated with @StrutsParameter. That is the same migration those members would have needed had the action not been ModelDriven. Model binding itself is unchanged.

Transition mode

requireAnnotations.transitionMode exempts depth-0 parameters so an application can enable requireAnnotations while it works through annotating. It was checked after the ModelDriven branch, so it never applied to a ModelDriven action at all. That was harmless while the exemption authorized everything, but it means the actions this change affects would have had no migration path — the one lever built for this situation was unreachable for exactly the actions that need it.

It is now checked first, so an application broken by this change can set requireAnnotations.transitionMode=true and its depth-0 action members keep binding, which is the same migration path any other action already has. This is also why no new opt-out flag is proposed.

Tests

Seven new cases in ParameterAuthorizerTest, covering the rejection, the annotated action member, the model property, the shadowed property, and the declared-on-neither escape.

Both new branches were mutation-checked rather than trusted because they passed:

  • removing the model-first check fails only the shadowing test — so the ordering is load-bearing
  • removing the declared-on-neither escape fails the new test and the pre-existing modelDriven_targetIsModel_allAuthorized — so that escape is what preserves existing behaviour

Green: core 3202, json 166, rest 124 (including ParameterAuthorizingModuleTest).

Note: a full-reactor mvn test currently fails to compile struts2-tiles-plugin (package org.apache.velocity.tools.view does not exist). That is pre-existing — it reproduces identically on unmodified main — and unrelated to this change, but it does mean the four modules after tiles were not exercised.

Related

WW-5697 / #1871 came from the same triage. Different cause, different fix; the two overlap only in that a ModelDriven action is the easiest way to reach both.

🤖 Generated with Claude Code

lukaszlenart and others added 2 commits August 27, 2026 08:16
isAuthorized returned true for every parameter name once the action implemented
ModelDriven. OGNL then resolves that name against the whole CompoundRoot, which
holds the model on top of the action, so authorization was decided about the
model while the write could land on the action. In effect the @StrutsParameter
requirement did not apply to a ModelDriven action's own members: an unannotated
setter declared on the action was bound, where the identical setter on a plain
action is rejected.

The exemption now covers what it was meant to cover. A property declared by the
model is exempt, since returning an object from getModel() declares it request
surface. A property declared by the action is subject to the annotation
requirement as usual. A property declared by neither is still allowed, because
it cannot be reaching a member of the action - that case is typically a model
bound through a custom OGNL property accessor, such as a Map-backed model, and
rejecting it would break those applications.

The model is checked first so that a model property shadowing an action property
still binds without an annotation, matching OGNL's own resolution against the
stack top.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The ModelDriven branch returned before the transition mode check, so
requireAnnotations.transitionMode never applied to a ModelDriven action. That
did not matter while the exemption authorized everything, but once it is scoped
to the model the action's own members are rejected, and those are exactly the
members transition mode exists to keep binding during migration.

Checking transition mode first gives the affected applications the same
migration path they would have on any other action.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lukaszlenart
lukaszlenart requested a lite review from Copilot August 27, 2026 16:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Scopes Struts’ ModelDriven parameter authorization exemption to properties declared on the model object, ensuring @StrutsParameter requirements still apply to action members and restoring transition-mode behavior for ModelDriven actions.

Changes:

  • Reorders and refines ModelDriven handling to authorize model-declared properties while applying annotation checks to action-declared properties.
  • Moves transition-mode depth-0 exemption earlier so it also applies to ModelDriven actions.
  • Adds regression tests covering ModelDriven action-member rejection/authorization, shadowing, “declared on neither” compatibility, and transition mode.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java Implements model-scoped ModelDriven exemption logic and reorders transition-mode evaluation.
core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizerTest.java Adds test cases validating the new ModelDriven scoping and transition-mode behavior.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…not by name

Copilot's review of #1872 found three ways the scoped exemption still let a
parameter through to the action's own members. All three reproduce.

Keying the exemption on the property name alone is not enough, because OGNL
walks the stack until an object actually accepts the assignment:

 - a getter-only property on the model cannot take a depth-0 parameter, so
   OGNL moves on and the action's unannotated setter takes it. Verified on a
   real value stack: the action's field ends up holding the value.
 - an inherited public field on the action was invisible to getDeclaredField,
   so the parameter counted as declared on neither model nor action and took
   the exemption meant for Map-backed models. OGNL sets inherited public
   fields as readily as declared ones.
 - a public static final namesake on the model cannot absorb a parameter
   either, and would have stood in for a real field.

declaresProperty therefore now asks what the object can bind at this depth -
the setter for a depth-0 parameter, the getter for a nested one, or a public
instance field - rather than whether the name appears anywhere.

Also rejects a parameter name that begins with a nesting character. It names
no root property, and computing one ran charAt(0) on an empty string.

Both changed methods are new in this PR, so their signatures are not yet API.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P9Pjt4rvb1ASASjSTHsUhL
lukaszlenart and others added 2 commits August 28, 2026 08:16
…y fallback

The remaining half of Copilot's first review comment on #1872: "class" was
exempted for a ModelDriven action, where the ordinary path rejects it.

Not for the reason the comment gives, though. OgnlUtil introspects with Object
as the stop class, so "class" never appears among the property descriptors at
all; it was not being matched as a read-only descriptor but taking the fallback
for a property declared on neither model nor action, which exists to let a
Map-backed model bind through its own OGNL accessor. That fallback is the wrong
home for it: "class" is not an unknown name, it is Object.getClass() on every
object alike, and the non-ModelDriven path rejects it for want of an annotation.

Rejected there rather than earlier, so a model or action that really does
declare a "class" property is still decided on its own terms.

This is defence in depth, not a live bypass. Navigating a class path is already
inert: java.lang.Class and java.lang.ClassLoader are both in the default
struts.excludedClasses, and SecurityMemberAccess refuses their members - checked
on a real value stack, where every class.* read returns null and every set has
no effect. Worth closing anyway, because the two paths disagreeing is the very
thing this ticket is about.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P9Pjt4rvb1ASASjSTHsUhL
…han lookup

SonarCloud failed the gate on javasecurity:S6173 — the request-derived property
name reaching Class.getField as a reflection lookup. In substance a false
positive: nothing is constructed or invoked, the Field is only inspected for its
modifiers. But the sink is avoidable at no cost, so avoid it.

Class.getFields() selects exactly the fields getField(name) searches — public,
declared and inherited — so scanning them and comparing the name is the same
decision without the name reaching a reflection API. It also reads consistently
with the property descriptor stream just above it.

Behaviour unchanged: core 3212, json 166, rest 124 all green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P9Pjt4rvb1ASASjSTHsUhL
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants